Add Action Client #262

jacobperron · 2019-01-24T02:09:49Z

Connects to #257

Added _rclpy_action extension module for interfacing rcl_action library and rclpy
Implemented ActionClient and integrated with Node as a Waitable
Added unit tests for ActionClient and ClientGoalHandle

I still need to go through and update the code documentation and perhaps add some more unit tests.

jacobperron · 2019-01-24T02:12:19Z

CI:

Linux
Linux-aarch64
macOS
Windows

sloretz

Partial review (I looked at tests). I'll start reviewing the implementation next.

rclpy/test/test_action_client.py

jacobperron · 2019-01-24T20:18:09Z

I still have to implement the synchronous methods (meant to do that prior to this PR).

sloretz

Another partial review, this time for the C code. There are not as many comments as it seems, they're mostly repetitive.

rclpy/src/rclpy/_rclpy_action.c

rclpy/src/rclpy/impl/common.h

With functions for creating and destroying action clients.

* Add boilerplate for send/take functions * Move common conversion function to shared header file (impl/convert.h) * Start implementing Waitable API * Add unit tests

Updated documentation and improved some error handling.

* Reset feedback member between each test * Used timed_spin() instead of spin_once()

* Better error handling * Move common typedef's to common.h * Return Python tuples from C functions instead of lists or custom Python object * Bugfix: pass Capsule objects for QoS profiles when creating an action client

jacobperron · 2019-01-25T20:53:42Z

Rebased on master.

@sloretz Thanks for the review so far!

I believe I've address most of your comments in 7ce5e74

…qos_policy()

sloretz · 2019-02-01T01:32:06Z

CI

Linux
Linux-aarch64
macOS
Windows

rclpy/rclpy/action.py

rclpy/rclpy/action_client.py

sloretz · 2019-02-01T01:53:54Z

rclpy/rclpy/action_client.py

+from unique_identifier_msgs.msg import UUID
+
+
+class ClientGoalHandle():


The C++ equivalent has a method to get a future for the goal result. Possible to implement that here?

sloretz · 2019-02-01T02:08:42Z

rclpy/rclpy/action_client.py

+        for seq, req_future in pending_requests.items():
+            if future == req_future:
+                try:
+                    del pending_requests[seq]


Is anything testing this? I would be surprised if it works since items() is a generator in python 3. See this SO post. Maybe it needs a request after the one being deleted to to see an error?

No explicit tests, but I tried a test sending multiple goals concurrently and still works. It would raise an exception, but the iteration stops on the first deletion because of the return statement below. Anyways, it's probably safer to cast the items to a list as suggested in the SO post.

We might consider doing the same for the service client code as well:

rclpy/rclpy/rclpy/client.py

Lines 70 to 76 in 0fafaec

for seq, req_future in self._pending_requests.items():

if future == req_future:

try:

del self._pending_requests[seq]

except KeyError:

pass

break

Cast to list: 04337c9

sloretz · 2019-02-01T02:17:14Z

rclpy/rclpy/action_client.py

+
+        return future
+
+    def get_result(self, goal_handle):


I would expect this method to only be on the goal_handle, like is done in rclcpp_action

rclpy/rclpy/action_client.py

sloretz · 2019-02-01T02:39:27Z

rclpy/rclpy/action_client.py

+
+        return future
+
+    def cancel_goal(self, goal_handle):


I would expect this to be a method on the goal handle.

In rclcpp_action this is done via the action client:

https://github.com/ros2/rclcpp/blob/c0a6b474d7a7507cf1403eaf47bb1dad919d31c9/rclcpp_action/include/rclcpp_action/client.hpp#L342

But, I do agree that it is more intuitive to move this to the goal handle. Since the implementation is closely tied to the ActionClient, what do you think about leaving these here as hidden methods and providing a facade in GoalHandle?

Also, after looking at rclcpp, I realized I'm missing functions for "cancel all goals" and "cancel goals before".

rclpy/src/rclpy/impl/common.h

Signed-off-by: Jacob Perron <[email protected]>

Now it acts as the users interface for canceling goals and getting goal results. Remove the unit tests specific to ClientGoalHandle since it is now closely coupled with the ActionClient and is sufficiently covered in the ActionClient unit tests. Signed-off-by: Jacob Perron <[email protected]>

jacobperron · 2019-02-01T22:45:41Z

@sloretz I've addressed all of your comments.

I've hidden the cancel_goal and get_result calls in ActionClient and added facades to GoalHandle (12907d7). It seemed like the easiest route considering the ActionClient is needed for making the outgoing/incoming requests.

Perhaps if we add the notion of a client goal handle to rcl and give it (and the server goal handle) more responsibility related sending/receiving cancel and result requests it will tie in more naturally at the rclpy and rclcpp layers. This is just a thought and outside the scope of this PR of course.

rclpy/rclpy/action/client.py

Signed-off-by: Jacob Perron <[email protected]>

sloretz

LGTM with green CI

jacobperron · 2019-02-02T01:08:40Z

Linux
Linux-aarch64
macOS
Windows

Increased to 400 seconds. Signed-off-by: Jacob Perron <[email protected]>

jacobperron · 2019-02-04T17:30:42Z

Increased test timeout:

Linux
Linux-aarch64
macOS
Windows

This reverts commit b6c6ca8.

Signed-off-by: Jacob Perron <[email protected]>

jacobperron · 2019-02-05T03:40:50Z

I'm not sure why, but the multi-threaded test was timing out on CI machines. I've disabled it and opened an issue (#268) to come back to.

Linux
Linux-aarch64
macOS
Windows

* Add rclpy_action module With functions for creating and destroying action clients. * Implement action client * Move common conversion function and typedefs to shared header file (impl/common.h) * Add tests using mock action server * Add action module for aggregating action related submodules * Extend Waitable API so executors are aware of Futures * Move check_for_type_support() to its own module Signed-off-by: Jacob Perron <[email protected]>

* Add Action Client (#262) * Add rclpy_action module With functions for creating and destroying action clients. * Implement action client * Move common conversion function and typedefs to shared header file (impl/common.h) * Add tests using mock action server * Add action module for aggregating action related submodules * Extend Waitable API so executors are aware of Futures * Move check_for_type_support() to its own module Signed-off-by: Jacob Perron <[email protected]> * Fix Executor not executing tasks if there are no ready entities in the wait set (#272) If a task is created, then trigger the Executors guard condition. This will wake any blocking call to `rcl_wait()`. In this scenario, no work is yielded, so we also have to move the check for 'in-progress' tasks into the wait loop. Added a unit test to reproduce the issue. This resolves an issue in some cases where the result response from the action server is never processed, therefore never reaching the action client. Signed-off-by: Jacob Perron <[email protected]> * Fix Node's reference to executor (#275) Previously, if a `Node` was added to an `Executor` using the executors `add_node` method, then nodes `executor` property would return `None`. Signed-off-by: Jacob Perron <[email protected]> * Abstract type conversions into functions (#269) * Abstract type conversions into functions This helps with readability and maintainability. Also eliminated use of assertions during conversions, defering to exceptions. * Move common C functions to a shared library 'rclpy_common' Signed-off-by: Jacob Perron <[email protected]> * Add ActionServer (#270) * Add Action server functions to extension module * Separated service related macros into separate request and response calls * Add server goal handle functions to extension module * Update Action extension module to use conversion functions * Add implementation of Python ActionServer * Handles goal and cancel requests, responds, and calls user-defined functions for executing goals. * Handle result requests * Handle expired goals * Publish goal status array and feedback * Add `handle_accepted_callback` to ActionServer Upon accepting a goal, the provided `handle_accepted_callback` is called with a reference to the goal handle. The user can then decide to execute the goal or defer. If `handle_accepted_callback` is not provided, then the default behavior is to execute the goal handle immediately. Signed-off-by: Jacob Perron <[email protected]> * Enable test using MultiThreadedExecutor (#280) Signed-off-by: Jacob Perron <[email protected]> * Guard against failed take when taking action messages (#281) Some middlewares (e.g. Connext and OpenSplice) send invalid messages to indicate an instance has been disposed which results in a 'failed take'. Signed-off-by: Jacob Perron <[email protected]>

jacobperron added the in progress Actively being worked on (Kanban column) label Jan 24, 2019

jacobperron mentioned this pull request Jan 24, 2019

Action implementation #257

Closed

2 tasks

jacobperron mentioned this pull request Jan 24, 2019

Actions ros2/ros2#583

Closed

24 tasks

sloretz reviewed Jan 24, 2019

View reviewed changes

rclpy/test/test_action_client.py Outdated Show resolved Hide resolved

rclpy/test/test_action_client.py Outdated Show resolved Hide resolved

jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jan 24, 2019

sloretz reviewed Jan 25, 2019

View reviewed changes

jacobperron added 17 commits January 25, 2019 12:31

Add rclpy_action module

b68d281

With functions for creating and destroying action clients.

[WIP] Implement action client

2270989

* Add boilerplate for send/take functions * Move common conversion function to shared header file (impl/convert.h) * Start implementing Waitable API * Add unit tests

Add tests using mock action server

33676c9

Add more of the Action extension module

eba5452

Implement send goal for action client

b217726

Remove action client as a waitable when destroyed

c3df614

Rename shared header file

4aa2480

Add ClientGoalHandle class and implement action client feedback

8336024

Implement cancel_goal_async

c75f66e

Implement get_result_async

6d5d3bb

Update ClientGoalHandle status from status array message

fb294d6

Lint

85758ab

Minor refactor

21f852a

Updated documentation and improved some error handling.

Fix action client test

1ba2a3f

* Reset feedback member between each test * Used timed_spin() instead of spin_once()

Implement synchronous action client methods

9c53704

Address review

7ce5e74

* Better error handling * Move common typedef's to common.h * Return Python tuples from C functions instead of lists or custom Python object * Bugfix: pass Capsule objects for QoS profiles when creating an action client

Add action module for aggregating action related submodules

b661737

jacobperron force-pushed the rclpy_action branch from 523c403 to b661737 Compare January 25, 2019 20:52

jacobperron added 2 commits January 25, 2019 17:13

Only destroy action client if node has not been destroyed

f2b238c

Improve error handling and add missing DECREF in rclpy_convert_to_py_…

9870847

…qos_policy()

sloretz reviewed Feb 1, 2019

View reviewed changes

jacobperron added 6 commits February 1, 2019 11:38

Move Action source files into 'action' directory

7cfb217

Signed-off-by: Jacob Perron <[email protected]>

Move check_for_type_support() to its own module

4505849

Signed-off-by: Jacob Perron <[email protected]>

Minor refactor of ActionClient

04337c9

Signed-off-by: Jacob Perron <[email protected]>

Add test for sending multiple goals with an ActionClient

ab8c6ae

Signed-off-by: Jacob Perron <[email protected]>

Fix DECREF calls

52afeca

Signed-off-by: Jacob Perron <[email protected]>

sloretz reviewed Feb 1, 2019

View reviewed changes

rclpy/rclpy/action/client.py Outdated Show resolved Hide resolved

sloretz reviewed Feb 1, 2019

View reviewed changes

rclpy/rclpy/action/client.py Outdated Show resolved Hide resolved

jacobperron added 2 commits February 1, 2019 15:35

action_server -> action_client

102de88

Signed-off-by: Jacob Perron <[email protected]>

Maintain weak references to GoalHandles in ActionClient

e2df735

Signed-off-by: Jacob Perron <[email protected]>

sloretz approved these changes Feb 2, 2019

View reviewed changes

Double test timeout

b6c6ca8

Increased to 400 seconds. Signed-off-by: Jacob Perron <[email protected]>

jacobperron added 2 commits February 4, 2019 13:06

Revert "Double test timeout"

cc2b886

This reverts commit b6c6ca8.

Disable failing test

c1422be

Signed-off-by: Jacob Perron <[email protected]>

jacobperron merged commit 6e086af into master Feb 5, 2019

jacobperron deleted the rclpy_action branch February 5, 2019 05:00

jacobperron removed the in review Waiting for review (Kanban column) label Feb 5, 2019

This was referenced Feb 6, 2019

Crystal Patch Release 2 ros2/ros2#647

Closed

Add ActionServer #270

Merged

jacobperron mentioned this pull request Feb 15, 2019

Crystal Patch Release 3 ros2/ros2#656

Closed

10 tasks

jacobperron mentioned this pull request Mar 7, 2019

Backport Python Actions #282

Merged

dirk-thomas mentioned this pull request Mar 21, 2019

ROS 2 Dashing Diademata ros2/ros2#607

Closed

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Action Client #262

Add Action Client #262

jacobperron commented Jan 24, 2019 •

edited

Loading

jacobperron commented Jan 24, 2019

sloretz left a comment

jacobperron commented Jan 24, 2019

sloretz left a comment

jacobperron commented Jan 25, 2019

sloretz commented Feb 1, 2019

sloretz Feb 1, 2019

sloretz Feb 1, 2019

jacobperron Feb 1, 2019

jacobperron Feb 1, 2019

sloretz Feb 1, 2019

sloretz Feb 1, 2019

jacobperron Feb 1, 2019

jacobperron commented Feb 1, 2019 •

edited

Loading

sloretz left a comment

jacobperron commented Feb 2, 2019

jacobperron commented Feb 4, 2019

jacobperron commented Feb 5, 2019

		from unique_identifier_msgs.msg import UUID


		class ClientGoalHandle():

	for seq, req_future in self._pending_requests.items():
	if future == req_future:
	try:
	del self._pending_requests[seq]
	except KeyError:
	pass
	break

Add Action Client #262

Add Action Client #262

Conversation

jacobperron commented Jan 24, 2019 • edited Loading

jacobperron commented Jan 24, 2019

sloretz left a comment

Choose a reason for hiding this comment

jacobperron commented Jan 24, 2019

sloretz left a comment

Choose a reason for hiding this comment

jacobperron commented Jan 25, 2019

sloretz commented Feb 1, 2019

sloretz Feb 1, 2019

Choose a reason for hiding this comment

sloretz Feb 1, 2019

Choose a reason for hiding this comment

jacobperron Feb 1, 2019

Choose a reason for hiding this comment

jacobperron Feb 1, 2019

Choose a reason for hiding this comment

sloretz Feb 1, 2019

Choose a reason for hiding this comment

sloretz Feb 1, 2019

Choose a reason for hiding this comment

jacobperron Feb 1, 2019

Choose a reason for hiding this comment

jacobperron commented Feb 1, 2019 • edited Loading

sloretz left a comment

Choose a reason for hiding this comment

jacobperron commented Feb 2, 2019

jacobperron commented Feb 4, 2019

jacobperron commented Feb 5, 2019

jacobperron commented Jan 24, 2019 •

edited

Loading

jacobperron commented Feb 1, 2019 •

edited

Loading